feat(agents): issue-585-feat-incidents-store-and-return-trace-id - #589

P2 — Permissive team == "" turns this into an existence/trace oracle for platform-scoped workflows.

The workflow name fed into the audit lookup is caller-controlled: a tenant developer can POST an incident with source: "argo-workflows" and a summary containing any https://workflows.mctl.ai/workflows/<ns>/<name> URL (or send workflow_name directly), then GET it. The distinct trace_unavailable values (audit_record_not_found vs no_traceparent_in_audit_record) plus a returned trace_id reveal whether an audit record exists for an arbitrary workflow name, and hand over its trace id.

auditEntryTenant(e) == "" means an AdminOnly platform-scoped operation — see the comment at internal/api/handlers_read.go:520-527, where GetWorkflowLogs deliberately denies non-admins in exactly this case. Here the empty-team branch grants access instead. Suggest mirroring the stricter gate:

return user.IsAdmin() || (team != "" && user.HasTenantAccess(team))

Tightening further to require team == a.Tenant for non-admins would also remove the cross-tenant probing surface, since the audit entry's team need not relate to the incident the caller was authorized for.

Fixed in 869866c. canReadAuditEntry lets a non-admin derive only from an audit entry whose team they belong to, so an empty-team (AdminOnly platform) entry is admin-only. I know you withdrew this P2 in 4252021227, since GetWorkflow already serves the traceparent. I kept the stricter gate anyway: the incident path has no reason to widen access, and argo incidents are posted to admins. The comment on the function now says so. Covered by TestCanReadAuditEntry and, through GetIncident with real non-admin users, by TestIncidentTrace_GetAsNonAdmin (b87e160). Mutation M12 fails that test.

P2 — Dedup updates trace_id but not workflow_name, so the two can end up describing different runs.

trace_id and workflow_name are two views of the same thing: the run that handled the incident. On the dedup path the new occurrence's trace_id overwrites the stored one, while workflow_name keeps occurrence #1's value (it is absent from the DO UPDATE SET list, and Update at line 209 does not touch it either).

Concretely, with a fingerprint that is stable across runs of the same failing template (the normal case for ArgoWorkflowFailed):

• occurrence ci: Bump docker/setup-buildx-action from 3 to 4 #1 posts workflow_name: wf-a and no trace -> row holds (wf-a, "")
• occurrence ci: Bump docker/build-push-action from 5 to 7 #2 posts workflow_name: wf-b, trace_id: T_b -> row holds (wf-a, T_b)

A later consumer reading workflow_name gets wf-a and a trace from wf-b. Symmetrically, if #1 carried a trace and #2 did not, deriveIncidentTrace would never run (reported wins) even though the stored trace is for a stale run.

Either move them together:

trace_id      = CASE WHEN EXCLUDED.trace_id <> '' THEN EXCLUDED.trace_id ELSE alerts.trace_id END,
workflow_name = CASE WHEN EXCLUDED.trace_id <> '' THEN EXCLUDED.workflow_name ELSE alerts.workflow_name END

or keep first-write-wins for both. Whichever is chosen, a store test asserting the dedup outcome for the pair would pin it down — the dedup path has no test for the new columns at all.

Fixed in 869866c. The pair now moves together on dedup. A new valid trace replaces both trace_id and workflow_name. A hit without a trace keeps both. While no trace is stored, a newer non-empty workflow_name replaces the older one, so derivation uses the latest run. This deviates from the proposal's "workflow_name immutable after create", which is recorded in the PR body. TestTraceID_Dedup pins all of these cases. Mutation M9 fails it.

P2 — The capture group swallows trailing punctuation, so a URL ending a sentence silently loses its trace.

([a-z0-9][a-z0-9.-]*) includes . and -, and the match is greedy to end of the run. For the very common summary shape where the URL terminates a sentence:

"Workflow failed: https://workflows.mctl.ai/workflows/argo-workflows/mctl-agents-implement-dae24112."

the capture is mctl-agents-implement-dae24112. — and IsDNS1123Subdomain rejects a trailing dot (the name must end alphanumeric), so line 100's guard fails and deriveIncidentTrace returns workflow_name_unknown for an incident whose workflow name is right there in the summary. A trailing - behaves the same way. The sole existing test ("summary url") uses an unpunctuated URL, so this is not covered.

Easiest fix is to anchor the end of the name instead of relying on the character class:

var workflowURLRe = regexp.MustCompile(`(?:^|\s)https://workflows\.mctl\.ai/workflows/[a-z0-9-]+/([a-z0-9](?:[a-z0-9.-]*[a-z0-9])?)`)

Worth adding table cases for a trailing ., a trailing ), and a URL in parentheses — note that (?:^|\s) also means "...failed (https://workflows.mctl.ai/...)" does not match at all, since ( is neither start-of-string nor whitespace.

Fixed in 869866c. The pattern is now (?:^|[\s(])…/([a-z0-9](?:[a-z0-9.-]*[a-z0-9])?). TestDeriveIncidentTrace_SummaryURLInProse covers a trailing ., a parenthesised URL, ). and a trailing -. Mutation M8, which restores the original pattern, fails it.

P3 — Update now clobbers trace_id from the in-memory value, which is a footgun for future callers.

All three current callers (UpdateIncident, AcknowledgeIncident, ResolveIncident) load existing via GetByPrefix first, so a.TraceID is always populated and nothing is lost today. But any future caller that builds a partial *Alert to update a couple of fields will silently blank a stored trace id, and the failure is invisible (no error, the column just becomes the empty string).

Consider making it non-destructive, matching the Create dedup policy — i.e. assign CASE WHEN <param> <> %s THEN <param> ELSE alerts.trace_id END instead of the bare parameter.

If clearing a wrong trace id is a wanted capability it needs an explicit path rather than falling out of a zero value: validIncidentTraceID maps both "absent" and "invalid" to the empty string, so a PATCH can never reset one anyway.

Fixed in 869866c: trace_id=CASE WHEN $12 <> '' THEN $12 ELSE trace_id END. A partial update that carries no trace now keeps the stored one. Asserted in TestTraceID_RoundTripsThroughEveryReadPath. Mutation M10 fails it.

P3 — The "exactly one is set" invariant does not hold on the create or patch response.

incident_trace.go:31 states that exactly one of trace_id / trace_unavailable is set, and the new MCP description promises callers that a missing trace_id always carries a reason. CreateIncident (201, line 97) and UpdateIncident (200, line 323) both return the stored alert without running deriveIncidentTrace and without setting a reason, so an argo-workflows incident created with no trace comes back with both fields empty — exactly the "this incident has no trace" misreading the ListIncidents loop at line 203 was added to prevent.

Setting TraceUnavailable = traceNotDerivedOnList on these two responses as well would make the contract uniform at no cost.

Separately, the shadowed-json.RawMessage decode trick here is subtle and has no HTTP-level test: TestValidIncidentTraceInputs exercises the validators directly, but nothing asserts that POST /api/v1/incidents with "trace_id": 123 still succeeds (rather than 400) and stores no trace, nor that "trace_source" / "trace_unavailable" sent by a client cannot reach the stored row. A small httptest case over the router would lock in the whole point of the shadowing.

Fixed in 869866c. Create, patch, ack and resolve responses go through labelStoredTrace. An argo incident without a trace says not_derived, and any other source says not_reported. TestIncidentTrace_WriteResponsesAreLabelled asserts the 201 and 200 bodies (b87e160). Mutation M13 fails it. The HTTP-level shadowing check, a numeric trace_id returning 201 with nothing stored, is in TestIncidentTrace_InvalidInputIgnoredNotRejected.

P3 — not_derived_on_list is also applied to incidents where derivation could never produce a trace.

For a non-argo-workflows incident deriveIncidentTrace returns not_reported — a terminal answer. Labelling those not_derived_on_list tells the caller (and the MCP client, per the new tool description: "call this tool to resolve it") to spend a GET /incidents/{id} that cannot possibly change the outcome. Gating on the source keeps the two reasons honest:

for i := range items {
    if items[i].TraceID != "" {
        continue
    }
    if items[i].Source == incidentSourceArgo {
        items[i].TraceUnavailable = traceNotDerivedOnList
    } else {
        items[i].TraceUnavailable = traceNotReported
    }
}

Fixed in 869866c. On the list, not_derived_on_list is only applied to argo incidents, and non-argo incidents without a trace get not_reported. The list test now seeds a non-argo untraced item. Mutation M11 fails it.

P3 — The store sets a field its own doc comment calls response-only.

types.go:82-84 says TraceSource is "response-only and never persisted", yet scanAlert derives it on every read path, including List and Summary. It is harmless (nothing writes it back, and deriveIncidentTrace resets it on the one path that matters) but it puts presentation logic in the persistence layer and gives List results a trace_source that no derivation produced.

Dropping the two lines and letting the handlers own both response-only fields would keep the layering consistent with the comment.

Fixed in 869866c. scanAlert no longer sets TraceSource; the handlers set both response-only fields.

P3 — Two nits on this const block.

1. The Argo UI hostname is hard-coded in workflowURLRe (line 45) and appears nowhere else in non-test code, so a different or additional Argo host (staging, a self-hosted install) silently breaks summary-based derivation with no way to configure it. Since producers are expected to send workflow_name directly, the regex is a legacy-row fallback — worth saying so in the comment, or sourcing the host from the same config the notifier uses.
2. incidentSourceArgo and incidentAuditTimeout sit in a block whose comment reads "trace_unavailable reasons. Exactly one is set when no trace id is returned." Splitting the timeout and the source constant into their own block would stop the comment from being wrong about three of its members.

Fixed in 869866c. The constants are split into reasons, sources and the source/timeout pair. The workflowURLRe comment says it is the fallback for rows posted without workflow_name, which is every argo incident today.

P2 — Still open from the previous round, and the new tests do not close it.

([a-z0-9][a-z0-9.-]*) includes . and matches greedily to the end of the run, so a URL that terminates a sentence captures the trailing punctuation:

"Workflow failed: https://workflows.mctl.ai/workflows/argo-workflows/mctl-agents-implement-dae24112."

captures mctl-agents-implement-dae24112., which IsDNS1123Subdomain rejects (a subdomain must end alphanumeric), so line 100 falls through to workflow_name_unknown for an incident whose workflow name is right there in the summary. A trailing - behaves the same way.

The new coverage uses daeSummary (incident_trace_handlers_test.go:37), copied verbatim from the production notifier — which happens to emit the URL last with no punctuation. So both the unit case and the new handler case exercise only the shape that already worked; the punctuated shape is still untested.

One-line fix, anchoring the end of the name rather than relying on the character class:

var workflowURLRe = regexp.MustCompile(`(?:^|[\s(])https://workflows\.mctl\.ai/workflows/[a-z0-9-]+/([a-z0-9](?:[a-z0-9.-]*[a-z0-9])?)`)

Note the leading class as well: (?:^|\s) means "... failed (https://workflows.mctl.ai/...)" does not match at all, since ( is neither start-of-string nor whitespace. Table cases for a trailing ., a trailing ), and a parenthesised URL would pin both down.

Already fixed in 869866c. This round reviewed e407e48, which was before that commit. See the reply on 4251856433.

P2 — This test now codifies the workflow_name/trace_id desync rather than resolving it.

The assertion is (workflow_name = "wf-first", trace_id = traceB): run A's name paired with run B's trace. That is exactly the inconsistency raised last round, now locked in by a test, with no comment anywhere explaining why the two fields are allowed to diverge.

Why it matters beyond tidiness: both fields are serialized on the incident response (types.go:79-80) and surfaced through mctl_get_incident. The feature exists so a responder can pivot incident -> workflow -> trace -> logs. A responder who reads workflow_name: wf-first and fetches that run's logs gets the wrong run, because the trace alongside it belongs to wf-second. During an incident that is a wrong answer, not a cosmetic one.

Two self-consistent options:

-- latest-wins for the pair
trace_id      = CASE WHEN EXCLUDED.trace_id <> '' THEN EXCLUDED.trace_id ELSE alerts.trace_id END,
workflow_name = CASE WHEN EXCLUDED.trace_id <> '' THEN EXCLUDED.workflow_name ELSE alerts.workflow_name END

or first-write-wins for both (drop trace_id from DO UPDATE SET).

If the current split really is deliberate — e.g. "workflow_name names the run that opened the incident, trace_id the most recent one" — that is defensible, but it needs saying in types.go next to the field docs, because nothing in the API tells a client the two refer to different runs. The test comment "workflow_name stays the original" describes the behaviour without justifying it.

Related: line 91 says "workflow_name is immutable", which reads as an enforced invariant. It is actually just that Store.Update does not list the column, so after.WorkflowName = "something-else" is silently dropped rather than rejected. Worth phrasing as "Update does not persist workflow_name" so a future reader does not rely on an immutability guarantee that is not implemented.

Already fixed in 869866c. This round reviewed e407e48. The test now asserts (wf-second, traceB), so the pair moves together. The "immutable" comment now says "Update does not persist workflow_name" (b87e160).

P3 — Correcting my own P2 from the previous round: this is not a privilege escalation. It is a consistency question plus a coverage gap.

I claimed the permissive team == "" branch made this a trace oracle for platform-scoped workflows. Checking the rest of the package, that does not hold:

• GetWorkflow (handlers_read.go:447) uses !user.IsAdmin() && team != "" && !user.HasTenantAccess(team) — the same tolerance of an empty team, for a caller-supplied workflow name, on the same non-admin route group (router.go:428).
• audit.RedactEntry masks on value length (>= 100) and credential markers (redact.go:22-36). A 55-character lowercase-hex traceparent matches neither, so GET /api/v1/workflows/{name} already returns audit.parameters.traceparent verbatim to any authenticated non-admin, for any workflow name including empty-team ones.

So the trace id reachable here is already reachable by the same caller through an existing endpoint, and the existence signal is too. No new capability is granted, and my suggested tightening would have made this stricter than the nearest precedent for no gain.

What is genuinely worth addressing:

1. Two gates coexist for the empty-team case — GetWorkflow permits, GetWorkflowLogs (handlers_read.go:520-531) denies, each with a comment explaining itself. This closure matches the former but says nothing. A one-line comment ("a trace id exposes no more than GetWorkflow already does; step logs are gated separately") would stop the next reader from relitigating it, as I just did.
2. The closure is the only line in the diff that makes an authorization decision, and no test exercises it with a real *auth.User. incident_trace_test.go covers deriveIncidentTrace with allow/deny stubs, and TestIncidentTrace_GetDerivesDae24112FromAudit only uses adminUser(). A non-admin case over an entry with a team_name they do not have (expect audit_record_forbidden) and one with no team (expect the derived trace) would cover both branches cheaply, using the withUser helper already in the package.

Thanks for the correction. I kept the stricter gate and documented why. Both branches are now covered with real *auth.User values through GetIncident in TestIncidentTrace_GetAsNonAdmin: own team gives the trace, another team gives audit_record_forbidden, and a platform op gives audit_record_forbidden.

P3 — Still open: not_derived_on_list is applied to incidents where derivation could never produce a trace, and the new test does not cover that case.

For a non-argo-workflows incident, deriveIncidentTrace returns not_reported — terminal. Labelling those not_derived_on_list tells the caller (and the MCP client, per the new tool description: "call this tool to resolve it") to spend a GET /incidents/{id} that cannot change the outcome.

TestIncidentTrace_ListMarksAbsenceWithoutAuditLookup seeds a source: "manual" incident, but gives it a TraceID, so it takes the continue path. The combination that exposes this — non-argo source and no stored trace — is the one shape the test does not seed.

for i := range items {
    if items[i].TraceID != "" {
        continue
    }
    if items[i].Source == incidentSourceArgo {
        items[i].TraceUnavailable = traceNotDerivedOnList
    } else {
        items[i].TraceUnavailable = traceNotReported
    }
}

Already fixed in 869866c (see the reply on 4251863225).

P3 — Still open: the "exactly one is set" invariant does not hold on the 201 create or the 200 patch response, and the new handler tests assert only the status code and the stored row, not the response body.

incident_trace.go:31 states that exactly one of trace_id / trace_unavailable is set, and the new MCP description promises that a missing trace_id always carries a reason. CreateIncident (201, line 97) and UpdateIncident (200, line 323) return the stored alert without running deriveIncidentTrace and without setting a reason, so an argo-workflows incident created with no trace comes back with both fields empty — the "this incident has no trace" misreading the ListIncidents loop was added to prevent.

TestIncidentTrace_InvalidInputIgnoredNotRejected checks rec.Code and then re-reads through store.Get, so the response bodies of both endpoints are still unasserted. Setting TraceUnavailable = traceNotDerivedOnList (or traceNotReported per the sibling comment) on these two responses would make the contract uniform, and asserting trace_unavailable on the decoded 201 body would lock it in — decodeIncident is already there for it.

Fixed in 869866c, and the response bodies are asserted in b87e160 (see the reply on 4251862373).

P3 — Still open, and the new store test now promotes this into the persistence layer's contract.

types.go:82-84 says TraceSource is "response-only and never persisted", yet scanAlert derives it on every read path including List and Summary. store_trace_test.go:29-39 (assertTrace) now asserts that trace_source == "reported" comes back from Create, Get, GetByPrefix and List — so the store is no longer just incidentally setting a presentation field, it is tested as owning one, directly against its own doc comment.

It is harmless at runtime (nothing writes it back, and deriveIncidentTrace resets both fields on the one path that matters). But either the store owns trace_source or the handlers do; right now the comment says one thing, the code does another, and the test pins the code. Dropping the two lines and setting the field in the handlers — with assertTrace checking only the two persisted columns — would make all three agree.

Also in this area, carried over: the Argo UI hostname is hard-coded in incident_trace.go:45 and appears nowhere else in non-test code, so a second or renamed Argo host silently breaks summary-based derivation with no way to configure it. Since producers are expected to send workflow_name directly, the regex is really a legacy-row fallback — worth saying so in the comment, or sourcing the host from the same config the notifier uses.

P3 — the new non-destructive CASE is right, but it leans on Postgres resolving an all-unknown operator to text, and CI cannot catch it if that ever fails.

The fix itself addresses the previous round's finding: all three callers keep their stored trace, and the new partial-update case in TestTraceID_RoundTripsThroughEveryReadPath pins it.

One caveat on the form. In CASE WHEN $12 <> '' both operands arrive untyped, so the statement parses only because Postgres falls back to text when every input to an operator is unknown. It does fall back, so this works today — but the failure mode if it ever did not is every incident PATCH/ack/resolve returning 500, and internal/alerts tests t.Skip without TEST_DATABASE_URL, which is the CI default in this repo. This SQL is new in 869866c (the previous commit had the bare trace_id=$12), so it is worth confirming the Postgres-backed run was repeated on this commit rather than only on e407e48.

Making the type explicit removes the dependence on the fallback:

trace_id=CASE WHEN $12::text <> '' THEN $12::text ELSE trace_id END

P3 — this branch blanks a known workflow_name, which is the correct reading of the new "they move together" policy but is the one dedup combination the test does not cover.

The pairing fix is good: TestTraceID_Dedup now covers new-trace-replaces-both, no-trace-keeps-both, and newer-name-while-untraced. What is left unexercised is EXCLUDED.trace_id <> '' AND EXCLUDED.workflow_name = '' — an occurrence that reports a trace but no workflow name. The first WHEN then assigns the empty EXCLUDED.workflow_name, so a row that held (wf-a, "") becomes ("", T_b): the incident keeps a trace but loses the workflow link it previously had in the response.

That is self-consistent (the alternative, keeping wf-a next to T_b, is exactly the desync this commit removes), so no SQL change is needed. But a future reader will ask, and the comment above the CASE does not say it. One line in TestTraceID_Dedup:

assertTrace(t, "a trace without a name clears the stale name", occur("argo-dedup-4", fp, traceA, "", 4), traceA, "")

plus a half-sentence in the doc comment ("a trace reported without a name clears the stale name rather than pairing it with the new trace") would make the choice deliberate rather than incidental.

P3 — the description enumerates the reason set, and 869866c added two values it does not mention.

The write responses (create, patch, ack, resolve) now return trace_unavailable: "not_derived", and a non-argo incident on a list response gets "not_reported" rather than "not_derived_on_list". Both are improvements over the previous round's "both fields empty", but an MCP client that reads this paragraph as the complete set now sees undocumented values — and the current text specifically tells it a list item carries not_derived_on_list, which is only true for argo incidents.

A sentence covering both keeps the tool description the single source of truth for the contract:

trace_unavailable is not_reported when no trace can exist for this incident source, not_derived_on_list for an argo incident on a list response (call this tool to resolve it), not_derived on a create/patch/ack/resolve response, or one of workflow_name_unknown / audit_unavailable / audit_record_not_found / audit_record_forbidden / no_traceparent_in_audit_record when the derivation ran and could not produce one.

P2 — For a deduped recurring argo incident, the derived trace is pinned to the first occurrence, so the responder gets the oldest run.

validIncidentWorkflowName leaves a.WorkflowName empty whenever the producer omits it — which the comment at incident_trace.go:52-53 says is every argo incident today. The Create dedup path (store.go:140-149) deliberately refreshes workflow_name from a newer occurrence while the row is untraced, but that clause can never fire if the producer never sends the field, and summary is not in the DO UPDATE SET list at all. So:

• run 1 fails -> row stored with run 1 summary (.../mctl-agents-implement-aaa11111), no trace, no name
• run 2 fails, same fingerprint, incident still open -> dedup bumps occurrence_count=2 and last_seen_at, summary and workflow name unchanged
• GET /incidents/{id} -> workflowURLRe reads run 1 out of the stale summary -> trace_id of run 1, labelled workflow_audit

Nothing in the response says the trace is two runs old: occurrence_count and last_seen_at point at run 2, trace_source claims a derived trace, and there is no trace_unavailable. For the use case this PR exists for — pivot incident -> trace -> Loki to see why it just failed — that is a wrong answer, not a missing one. The dedup window is real: the ON CONFLICT predicate only excludes resolved/suppressed, so every retry while the incident is open lands here.

Smallest in-scope fix is to fill the field the dedup clause already handles, right here:

a.WorkflowName = validIncidentWorkflowName(body.WorkflowName)
if a.WorkflowName == "" {
    a.WorkflowName = workflowNameFromSummary(a.Summary) // the workflowURLRe match, factored out
}

Then occurrence 2 supplies wf-run2, the alerts.trace_id = '"' AND EXCLUDED.workflow_name <> '"' clause promotes it, and deriveIncidentTrace prefers a.WorkflowName over the stale summary. It also makes the stored row self-describing instead of depending on a regex over prose at read time.

Either way this needs a test: TestTraceID_Dedup covers the columns but never the derivation, and TestIncidentTrace_GetDerivesDae24112FromAudit uses a single-occurrence row, so no test exercises GET on a deduped argo incident. If the staleness is accepted instead, it should be stated in the trace_source docs ("workflow_audit names the run recorded in the incident, which for a recurring incident is the first occurrence").

Fixed in f2472b6. CreateIncident now stores the workflow name parsed from the summary for argo incidents when the producer sends none (workflowNameFromSummary, shared with derivation). The dedup clause carries workflow_name forward on each occurrence while no trace is stored, so derivation follows the newest run. Covered by TestIncidentTrace_DedupedArgoIncidentFollowsNewestRun, which posts run1 and run2 under one fingerprint and expects run2's trace. Mutation check: removing the fallback makes that test fail with derived trace_id=008ad897... workflow_name=<nil>, want the newest run.

P3 — The first WHEN arm blanks a stored workflow_name, and that case is the one the new dedup test does not cover.

WHEN EXCLUDED.trace_id <> '' THEN EXCLUDED.workflow_name assigns the incoming name unconditionally, including when it is empty. So an occurrence that carries a trace but no workflow name erases a name the row already had:

That is the shape a producer will have as soon as it starts sending trace_id (the obvious first step) without also sending workflow_name. Arguably it keeps the pair honest — the name of run B genuinely is unknown — but the row then names no workflow at all while summary still links run A, so the "move together" invariant is not actually achieved either; it just drops the one field that was accurate.

TestTraceID_Dedup covers (trace, name), ('', name) and ('', ''), but never (trace, ''), so whichever behaviour is intended is currently unpinned. If keeping wf-a is preferred, the arm is the same CASE shape used for trace_id:

workflow_name = CASE
    WHEN EXCLUDED.workflow_name <> '' THEN EXCLUDED.workflow_name
    WHEN EXCLUDED.trace_id <> '' THEN ''
    ELSE alerts.workflow_name END

...or, if blanking is intended, add the row to the test table so a future reader knows it was a decision.

Intended, and now documented and tested in f2472b6. trace_id and workflow_name describe the same run, so a new trace with no name must not stay paired with the previous run's name. There is a comment on the clause, and a new TestTraceID_Dedup case "a trace without a name clears the stale name". Mutation check: guarding the first arm with AND EXCLUDED.workflow_name <> '' makes that case fail.

P3 — The description is now out of date with the two labelling changes in this commit.

"List results carry trace_unavailable not_derived_on_list" was true of the previous commit, which labelled every traceless item that way. After labelStoredTrace, a non-argo item on the list gets not_reported instead (asserted by TestIncidentTrace_ListMarksAbsenceWithoutAuditLookup), and that one is terminal — calling mctl_get_incident on it cannot change the answer. As written the description sends the model to spend a GET on exactly the items where it is pointless, which is the cost the not_reported split was introduced to avoid.

The new not_derived reason returned by the create/patch/ack/resolve responses is also undocumented anywhere a client can see it.

Suggest naming the two actionable reasons explicitly, e.g.:

trace_unavailable gives the reason a trace id is absent. "not_derived_on_list"
(list results) and "not_derived" (write responses) mean the derivation was not
run — call this tool to resolve them. Every other reason, including
"not_reported", is terminal.

Fixed in f2472b6. The mctl_get_incident description now lists every trace_unavailable value and says when each one appears (list vs get).

P3 — Non-destructive now (good), but it closes the only door for correcting a wrong trace id.

Between this CASE and validIncidentTraceID mapping both "absent" and "invalid" to the empty string, there is no representable request that clears trace_id. If a producer ever posts a syntactically valid but wrong trace (a parent trace, a trace from the wrong leg), the incident is stuck reporting it as trace_source: "reported" forever — and reported beats derivation in deriveIncidentTrace, so the correct derivable trace stays hidden.

Not worth blocking on, and the empty-means-keep rule is the right default for the partial-update callers. Worth a line in the TraceID doc comment (types.go:78-81) saying the value is write-once-per-value and cannot be reset through the API, so the next person does not go looking for the clear path.

Same for workflow_name: it is absent from this SET list and from the UpdateIncident patch struct, so a row created without one can only ever gain one through a dedup hit. The store_trace_test.go:84-85 comment now records that accurately, but the API surface does not.

Acknowledged and documented in f2472b6 (types.go doc on TraceID/WorkflowName). No API path clears a stored trace, and PATCH never sets workflow_name. A wrong trace can be corrected by PATCHing a new valid trace_id or by a newer occurrence that carries one. Supporting an explicit clear would need a tri-state field in the PATCH body, which is out of scope here. The $12::text cast is applied in the same commit.